Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make raw handle traits safe to implement #131

Closed
wants to merge 1 commit into from

Conversation

notgull
Copy link
Member

@notgull notgull commented Jul 2, 2023

Here's a thought experiment that we should at least consider before releasing v0.6 (#125). Since we now have borrowed display/window handles, with conditions on them enforced via their borrow_raw methods, what if we made the raw traits safe to implement? They're unsafe to implement now, as contrasted with AsRawFd in the standard library, which are safe.

The primary downside would be the introduction of a footgun where users try to use HasRawWindowHandle in their APIs are they do now, unaware that they can return undefined window handles.

@i509VCB
Copy link

i509VCB commented Jul 16, 2023

I'm skeptical there is a benefit to this. HasRawDisplayHandle and HasRawWindowHandle imply that the implementer either owns the underlying handle(s) or borrows the handles (which would require following the contract of the API).

Regarding the comparison to AsRawFd I feel HasRawWindowHandle differs in the semantics. If an fd is closed and you use the fd somewhere, you'll get some errno or io::Error return value. Whereas with raw window/display handles you are likely passing around a pointer which on some platforms could be dangling if the owner of the handles is freed.

@ids1024
Copy link
Member

ids1024 commented Jul 16, 2023

They're unsafe to implement now, as contrasted with AsRawFd in the standard library, which are safe.

The safety, and possibly the existence, of this trait is arguably just a relic of it predating IO safety. And the fact that can't be changed.

Is HasRawWindowHandle still needed? Or would it be better for all implementers to just implement HasWindowHandle, and get a raw window handle from that, as needed? Is there any reason to implement HasRawWindowHandle but not HasWindowHandle?

If an fd is closed and you use the fd somewhere, you'll get some errno or io::Error return value.

Not necessarily. The fd may have been re-used, and may now point to a different file description. Which may be mapped into memory, so you may cause undefined behavior by writing to it.

So you might get an IO error, but that can also be unsound, and that is basically the reason for the IO safety rules and types.

@i509VCB
Copy link

i509VCB commented Jul 16, 2023

If I recall the end goal of the regular WindowHandle type is to make the whole process of getting a softbuffer/gl context/etc safe. However that would need a lifetime on context types? So a Display<'a> for example. But some use cases you pretty much can't have a lifetime, (wayland-rs Dispatch: 'static is very guilty of this). I guess you could transmute a Display<'a> to a Display<'static> but that would require writing transmutes and you are heavily discouraged from doing that plus it's a UB nightmare waiting to happen.

I guess ouroboros could be a workaround to the "context needs to be owned with the display" scenario but using ouroboros feels unclean.

@notgull notgull closed this Jul 16, 2023
@notgull notgull deleted the notgull/safe-hrwh branch July 16, 2023 04:55
@notgull
Copy link
Member Author

notgull commented Jul 16, 2023

I agree with the rationale presented here, so I’ll close this PR.

The intended solution to the problem of a borrowed window handle having a lifetime parameter is to pass around the T: HasWindowHandle itself. I've applied this pattern to my conversions so far, see rust-windowing/softbuffer#132, gfx-rs/wgpu#3932 and rust-windowing/glutin#1582. This way you can use WindowHandle<'a>, Rc<Window> and Arc<Window> in all of these places without any issues.

ouroboros is sketchy, I agree.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants